Skip to content

refactor: use reflect.TypeFor#682

Closed
box4wangjing wants to merge 2 commits into
singnet:masterfrom
box4wangjing:master
Closed

refactor: use reflect.TypeFor#682
box4wangjing wants to merge 2 commits into
singnet:masterfrom
box4wangjing:master

Conversation

@box4wangjing

Copy link
Copy Markdown

Try to use better api reflect.TypeFor instead of reflect.TypeOf when we have known the type.
More info golang/go#60088

Inspired by #653 and replace all.

Signed-off-by: box4wangjing <box4wangjing@outlook.com>
@box4wangjing

Copy link
Copy Markdown
Author

@semyon-dev Hi, Could you please review this PR at your convenience? Thank you very much.

@semyon-dev semyon-dev self-requested a review May 22, 2026 10:07

@semyon-dev semyon-dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration itself looks safe and preserves the existing value-type semantics (T instead of *T).

One thing worth double-checking is that TypedAtomicStorageImpl intentionally expects non-pointer reflected types, while runtime usage operates on pointers (*ModelKey, *ModelData, etc.). This behavior already existed before the refactor, so the PR does not introduce it, but it's an important invariant for future changes.

@semyon-dev

Copy link
Copy Markdown
Member

One small note: the PR description says "replace all", but there are still several handwritten storage-related usages of reflect.TypeOf(T{}) remaining (for example in payment_channel_storage.go, prepaid_storage.go, and license_storage.go).

The remaining .pb.go generated files are probably fine to leave untouched, but it may be worth either:

  • updating the PR description to clarify the scope, or
  • including the remaining storage-layer replacements for consistency.

@semyon-dev semyon-dev added the help wanted Extra attention is needed label May 25, 2026

@kiruxaspb kiruxaspb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reject notes:

The PR says "replace all", but there are still a few storage-related reflect.TypeOf(T{}) usages left (e.g. in payment_channel_storage.go, prepaid_storage.go, and license_storage.go). Either update the description or include those replacements for consistency.

@semyon-dev semyon-dev closed this Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants